fix(gerrit): not auto-merging if Code-Review changed from +2 to +1 #32818

felipecrs · 2024-11-29T22:48:30Z

Changes

The checkIfApproved function was not working as expected when someone already had published a Code-Review +1 vote. It was wrongly assuming that the change was approved in such case, while the change should only be approved if the change had a Code-Review +2 vote.

Note that when o=DETAILED_LABELS is set, the change response from Gerrit API no longer contains the content from o=LABELS, meaning it should only ever be used in this case (when looking for the approvals), otherwise it would break existing code.

This fix also ensures that the change will be auto-approved again during the auto-merge process, in case the Code-Review +2 was changed to +1 by some CI check using the same account as Renovate, like SonarQube (which posts Code-Review +1).

Finally, this fix will improve the approval logic for when the change is initially created or updated. It will avoid two API calls in each case.

Context

I self-host Renovate and I noticed that auto-merge was not working as expected. I realized it was caused by this issue, because for a given project we have here, the same user/robot also votes the change with a Code-Review +1 vote when SonarQube analysis is successful (which is a very known pattern).

Documentation (please check one with an [x])

I have updated the documentation, or
No documentation update is required

How I've tested my work (please select one)

I have verified these changes via:

Code inspection only, or
Newly added/modified unit tests, or
No unit tests but ran on a real repository, or
Both unit tests + ran on a real repository

viceice

conflicted

…ix-auto-merge-if-plus-one

felipecrs · 2024-12-01T15:05:07Z

I just realized this function is also implemented by the GitLab provider, and there it doesn't check for whether it's approved already or not, meaning it will probably issue an error, which is not desired.

I'll think about it.

felipecrs · 2024-12-01T16:43:01Z

@viceice, @rarkins, can you please help me understand this failed check?

❌ Error: Snapshots have been added in this PR. Use toMatch instead.

felipecrs · 2024-12-01T16:46:40Z

I've refactored the other two platforms (Azure and GitLab) that also use autoApprove to account for these improvements as well:

As last resort, the PR will be attempted to be auto-approved before auto-merge, in case the approval got dismissed after it was initially published for whatever reason.
It will no longer attempt to auto-approve if the PR is already approved

Note I do not use Azure or GitLab. I coded these functions based on their documentation. But I believe there is no big risk involved, given it will still attempt to auto-approve in case the is approved condition doesn't match.

lib/workers/repository/update/pr/automerge.ts

lib/modules/platform/gitlab/index.ts

lib/workers/repository/update/pr/automerge.ts

felipecrs · 2024-12-02T17:02:11Z

@rarkins, it's presumptuous from my part that this would be of any help to the existing platforms. I will revert the changes to Azure and GitLab, and focus on Gerrit, which is what I use and know this would make sense for.

This reverts commit c679c28.

…ser" This reverts commit 5eb2846.

…ix-auto-merge-if-plus-one

lib/modules/platform/gerrit/client.ts

lib/modules/platform/gerrit/index.spec.ts

lib/workers/repository/update/pr/automerge.spec.ts

lib/modules/platform/gerrit/scm.ts

…ix-auto-merge-if-plus-one

felipecrs · 2024-12-29T15:56:46Z

@viceice, sorry for taking so long. I thought it would be harder to address your comments, but they were in fact straightforward and helpful. Thank you very much.

EDIT: working on the failed coverage.
EDIT 2: done.

felipecrs · 2025-01-15T14:38:40Z

@viceice, this is a kind ping. :)

viceice

conflicted

viceice

otherwise LGTM

viceice · 2025-01-24T11:40:52Z

lib/workers/repository/update/pr/automerge.ts

@@ -69,6 +69,10 @@ export async function checkAutoMerge(
      prAutomergeBlockReason: 'PlatformNotReady',
    };
  }
+  if (config.autoApprove && platform.approvePrForAutomerge) {
+    logger.debug('Auto-approving PR for automerge');
+    await platform.approvePrForAutomerge(pr.number);


it this a noop if it's already approved? see other open conversation

But the PR could have been un-approved after initial approve. See the PR body's Context:

I self-host Renovate and I noticed that auto-merge was not working as expected. I realized it was caused by this issue, because for a given project we have here, the same user/robot also votes the change with a Code-Review +1 vote when SonarQube analysis is successful (which is a very known pattern).

SonarQube analysis in such case happens during CI, after Renovate has initially approved the PR upon creation or update.

This extra-check will account for cases where the PR had its vote downgraded from +2 to +1.

I believe this is a special case that can only happen with Gerrit. On GitHub and Gitlab, there's no such concept of approval level. It's either approved or not, by a given user.

However, in Gerrit, "approvals" can either be +2 (highest, needed to merge), or +1 (not enough to merge). And a user, can have its vote decreased from +2 to +1.

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 5b42180 to 4028843 Compare November 29, 2024 23:14

felipecrs marked this pull request as draft November 29, 2024 23:33

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 4028843 to 06ecde1 Compare November 29, 2024 23:53

felipecrs marked this pull request as ready for review November 29, 2024 23:53

fix(gerrit): not auto-approving if change already had a Code-Review +1

46f8eed

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 06ecde1 to 46f8eed Compare November 29, 2024 23:58

chore(gerrit): improve auto-approve on new patchset workaround

107a298

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 82fe8e1 to 107a298 Compare November 30, 2024 01:52

felipecrs marked this pull request as draft November 30, 2024 06:18

chore(gerrit): try auto-approve right before auto-merge

6ac7b9e

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from dcc53d7 to 6ac7b9e Compare November 30, 2024 07:12

Merge branch 'main' into fix-auto-merge-if-plus-one

6ffa5b6

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 4817e82 to 6ffa5b6 Compare November 30, 2024 07:18

felipecrs marked this pull request as ready for review November 30, 2024 07:24

rarkins previously approved these changes Nov 30, 2024

View reviewed changes

viceice reviewed Dec 1, 2024

View reviewed changes

Merge branch 'main' of https://github.com/renovatebot/renovate into f…

7877331

…ix-auto-merge-if-plus-one

felipecrs dismissed rarkins’s stale review via 7877331 December 1, 2024 14:59

felipecrs marked this pull request as draft December 1, 2024 15:29

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 56bd600 to 761106b Compare December 1, 2024 16:38

test(gerrit): fix approvePr coverage

1d8daf5

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 761106b to 45763e9 Compare December 1, 2024 16:40

felipecrs marked this pull request as ready for review December 1, 2024 16:43

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 45763e9 to 5a0955c Compare December 1, 2024 16:49

felipecrs marked this pull request as draft December 1, 2024 16:51

chore(gerrit): still approve if change was approved another user

5eb2846

felipecrs force-pushed the fix-auto-merge-if-plus-one branch from 5a0955c to ab9907a Compare December 1, 2024 17:06

felipecrs added 3 commits December 1, 2024 17:28

test(gerrit): fix lint in client.spec.ts

bd05816

test(automerge): add test for auto approve before merging

ac30f2b

Merge branch 'main' into fix-auto-merge-if-plus-one

539c12f

rarkins reviewed Dec 2, 2024

View reviewed changes

lib/workers/repository/update/pr/automerge.ts Outdated Show resolved Hide resolved

rarkins requested changes Dec 2, 2024

View reviewed changes

lib/modules/platform/gitlab/index.ts Outdated Show resolved Hide resolved

lib/workers/repository/update/pr/automerge.ts Outdated Show resolved Hide resolved

felipecrs marked this pull request as draft December 2, 2024 17:02

felipecrs added 4 commits December 2, 2024 17:19

Revert "feat(gitlab,azure): try approving before auto-merge"

7923d5b

This reverts commit c679c28.

chore: rename approvePr to approvePrForAutomerge

c4c81e3

Revert "chore(gerrit): still approve if change was approved another u…

4dfc817

…ser" This reverts commit 5eb2846.

chore(gerrit): improve approve method to avoid some API calls

bb317ce

felipecrs changed the title ~~feat(autoApprove): auto-approve PR before auto-merge attempt if needed~~ fix(gerrit): not auto-merging if Code-Review vote was downgraded Dec 2, 2024

felipecrs changed the title ~~fix(gerrit): not auto-merging if Code-Review vote was downgraded~~ fix(gerrit): not auto-merging if Code-Review changed from +2 to +1 Dec 2, 2024

felipecrs added 3 commits December 2, 2024 17:36

Merge branch 'main' of https://github.com/renovatebot/renovate into f…

b1ce02b

…ix-auto-merge-if-plus-one

chore: fix type import

53743b2

chore: remove spurious comment

e0b6f8d

felipecrs requested a review from rarkins December 2, 2024 18:32

felipecrs marked this pull request as ready for review December 2, 2024 18:32

viceice reviewed Dec 6, 2024

View reviewed changes

felipecrs added 2 commits December 29, 2024 12:41

Merge branch 'main' of https://github.com/renovatebot/renovate into f…

74137a8

…ix-auto-merge-if-plus-one

Address review comments

b93ee7a

felipecrs requested a review from viceice December 29, 2024 15:53

Add tests for approvePrForAutomerge

add6ca5

felipecrs mentioned this pull request Dec 29, 2024

refactor(gerrit): remove deprecated source branch as hashtags support #33329

Draft

6 tasks

viceice reviewed Jan 17, 2025

View reviewed changes

Merge

933f13d

felipecrs requested a review from viceice January 17, 2025 18:26

viceice reviewed Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(gerrit): not auto-merging if Code-Review changed from +2 to +1 #32818

fix(gerrit): not auto-merging if Code-Review changed from +2 to +1 #32818

felipecrs commented Nov 29, 2024 •

edited

Loading

viceice left a comment

felipecrs commented Dec 1, 2024

felipecrs commented Dec 1, 2024

felipecrs commented Dec 1, 2024 •

edited

Loading

felipecrs commented Dec 2, 2024

felipecrs commented Dec 29, 2024 •

edited

Loading

felipecrs commented Jan 15, 2025

viceice left a comment

viceice left a comment

viceice Jan 24, 2025

felipecrs Feb 1, 2025

felipecrs Feb 1, 2025

fix(gerrit): not auto-merging if Code-Review changed from +2 to +1 #32818

Are you sure you want to change the base?

fix(gerrit): not auto-merging if Code-Review changed from +2 to +1 #32818

Conversation

felipecrs commented Nov 29, 2024 • edited Loading

Changes

Context

Documentation (please check one with an [x])

How I've tested my work (please select one)

viceice left a comment

Choose a reason for hiding this comment

felipecrs commented Dec 1, 2024

felipecrs commented Dec 1, 2024

felipecrs commented Dec 1, 2024 • edited Loading

felipecrs commented Dec 2, 2024

felipecrs commented Dec 29, 2024 • edited Loading

felipecrs commented Jan 15, 2025

viceice left a comment

Choose a reason for hiding this comment

viceice left a comment

Choose a reason for hiding this comment

viceice Jan 24, 2025

Choose a reason for hiding this comment

felipecrs Feb 1, 2025

Choose a reason for hiding this comment

felipecrs Feb 1, 2025

Choose a reason for hiding this comment

felipecrs commented Nov 29, 2024 •

edited

Loading

felipecrs commented Dec 1, 2024 •

edited

Loading

felipecrs commented Dec 29, 2024 •

edited

Loading